Regarding test flakiness, one contributor could be where you get the info that you use in your assertions. Using internalCluster().clusterService() doesn't necessarily give you the most up to date value since it might not fetch the instance from the current master node. This is documented on that method, but want to make sure you have consider this random behaviour that this utility can have. You can fetch the master node instance by providing the master node name or similar to the test in TransportMasterNodeActionIT, which I assume this is partially based on, use a master client.

Having tried that out briefly, I think the test failure is on longer due to the latch you mentioned but rather log expectation assertions. Hope that hleps!

(at least based on 20-something runs that used to get 8-10 failures in the latch check, which now only fails once later at the log expectation assertion)

I think this'd be much simpler implemented with ClusterServiceUtils.addTemporaryStateListener. Not sure why we didn't do so when first written - I think it must have evolved to this from something more complex.

Good catch, have updated in a668ab1

Hmm I don't think you need to pull this one up to the base class, it only has one caller.

Updated in a668ab1

Do you need a separate method here? In the new tests I think we can just wait for the term to increase, as observed by any node - no need to worry about which node is master or anything so fiddly.

Also this one is only called from one place. I think it's a premature optimization to generalize these two test suites like this when they have so little in common.

Updated in a668ab1

The new tests don't need 5 master nodes, and indeed having 5 (rather than 3) makes the situation unnecessarily complex.

Updated in a668ab1

Nice, I think that makes sense and is much simpler than I anticipated.

This doesn't quite work tho, because we apply a cluster state when the master fails too. The state we apply in that case differs from the last committed state only in the values of things which are local properties of the state, i.e. which do not have strong consistency guarantees, such as DiscoveryNodes#masterNodeId. I think we need to keep the connection open until we've applied the next committed state (i.e. one with a non-null master).

I did see the new tests fail in this way when run in a loop, but unfortunately I didn't have things configured to collect a build scan. I'm trying again.

Also we should only insert this listener when the update fails with a FailedToCommitClusterStateException - for all other outcomes there's no need to wait for another state to be committed, indeed we cannot expect another state to be committed any time soon if the cluster is stable.

This makes sense. I've changed this logic to only close the connection when a committed state is seen, but how would I know whether a FailedToCommitClusterStateException is thrown?

rather than using ActionListener#runBefore you'd typically capture the exception with ActionListener#delegateResponse. But in this case we're already capturing the success response with ActionListener#delegateFailure so it's probably best to just create a completely new ActionListener that does the right thing with both success and exceptional responses.

Updated in a668ab1

Stray logging leftover from debugging? I don't think we want an INFO log here, it will raise support cases asking what it means.

Updated in a668ab1

This is only used in tests, I don't think there's a need to have it in the production codebase.

Updated in a668ab1

This isn't correct, sometimes the new node will be of the form node_tN. There may be other possibilities too even today, I'm not sure, and we certainly don't know what'll happen in future. More generally, this is heavily overspecifying the conditions we actually want, and coupling this test to the implementation details of how nodes are named in the cluster. If we changed the format of these messages at all, we'd have to update this code too.

I think we should be asserting that we expect to see no messages at all from NodeJoinExecutor at level WARN. The content isn't really relevant.

Good point, I have updated in cc67155

We also need to fail the join listener and remove the cluster-state listener if the last-applied state is committed (i.e. has a master node) and doesn't include the joining node.

Does it work to call clusterStateListener.clusterChanged(new ClusterChangedEvent("", clusterService.state(), clusterService.state()))?

Does it work

Hmm no, not quite: that might complete ll twice which is generally something we should try and avoid. It turns out that as things are implemented today ll will be a SubscribableListener which has well-defined semantics when completed multiple times:

However we can't rely on that being true in future, it's not guaranteed that ll will always be a SubscribableListener in this context. I'd prefer we explicitly deduplicated this work e.g. by creating another SubscribableListener.

However we can't rely on that being true in future

I contemplated making it not be true by adding a check that these listeners are not completed multiple times:

diff --git a/server/src/main/java/org/elasticsearch/action/support/SubscribableListener.java b/server/src/main/java/org/elasticsearch/action/support/SubscribableListener.java
index 9eddbb55b776..b7c8c4a28279 100644
--- a/server/src/main/java/org/elasticsearch/action/support/SubscribableListener.java
+++ b/server/src/main/java/org/elasticsearch/action/support/SubscribableListener.java
@@ -135,7 +135,7 @@ public class SubscribableListener<T> implements ActionListener<T> {
      */
     public static <T> SubscribableListener<T> newForked(CheckedConsumer<ActionListener<T>, ? extends Exception> fork) {
         final var listener = new SubscribableListener<T>();
-        ActionListener.run(listener, fork::accept);
+        ActionListener.run(ActionListener.assertOnce(listener), fork::accept);
         return listener;
     }

However, on reflection this seems unnecessarily strict and indeed it causes the SubscribableListener test suite to fail because we actually already assert that newForked and andThen receive the returned listener instance, which is therefore safe to complete more than once. I think it's best to document this fact, see #133391, and then we can rely on it here too (so disregard my previous message)

nit: slight preference for this being two assertThat calls: if this fails, we get no feedback about why, whereas if we use assertThat twice then we'll be able to see whether it was the master that changed (and it'll identify the new master) or whether the term didn't increase.

It seems like it'd be simpler to call clusterStateListener.clusterChanged directly here, constructing a "fake" ClusterChangedEvent to carry the current cluster state. I think we want the same logic both ways: particularly if the node is for some reason in the current cluster state at this point then we can complete ll successfully too.

Btw this test is a really good discriminator of the improvement, it failed for me 9 out of 10 times I ran it having reverted the behaviour change in Coordinator, and passed 10 out of 10 with the change in place. Great stuff.

Why have this within the if branches? It's on both branches so we call it either way, and I don't see a need to call it after calling nodeExists or anything like that?